Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Rust FFI #10: Remove pact broker #302

Closed
wants to merge 55 commits into from

Conversation

tienvx
Copy link
Contributor

@tienvx tienvx commented Mar 13, 2023

This PR follow suggestion from @mefellows that:

  • Don't embed cli (from Ruby Standalone) into Pact-PHP
  • Recommend most users of Pact to pick your client language (e.g. PHP) and then download the CLI tools to do the CI/CD integration

One of the reason is: The Ruby Standalone has windows long file path and not working on a bunch of OS/Arch combinations. So bundling it into Pact PHP will mean Pact PHP won’t be installable on those same systems

@mefellows will give further explanation if needed.

I think this suggestion make sense because:

  • This speed up the tests, because if we register more than 1 listener, they will publish pacts multiple times.
  • Sometime we just want to run the tests, but don't want to publish pacts (e.g. local development?)
  • Let imagine there are 3 test suites (Unit, Contract, Integration) running in this specific order. If Unit and Contracts tests are passed, the listener publish pacts, but then Integration tests failed. Then I think we published some pacts that we don't really want.

For Github Actions it's quite easy to add a step to publish pacts while still follow other recommendation (like include commit SHA in the version, use branch instead of tag):

    - name: Publish pacts
      uses: pactflow/actions/publish-pact-files@auto_detect_version_branch
      env:
        pactfiles: tests/Contract/pacts
        PACT_BROKER_BASE_URL: ${{ secrets.PACT_BROKER_BASE_URL }}
        PACT_BROKER_TOKEN: ${{ secrets.PACT_BROKER_TOKEN }}

This PR depend on #284. Please review it first.

@tienvx tienvx force-pushed the remove-pact-broker branch 2 times, most recently from dd90f88 to affb5cd Compare March 16, 2023 04:06
@tienvx tienvx force-pushed the remove-pact-broker branch 2 times, most recently from db57b54 to d74d9b3 Compare March 21, 2023 05:15
@tienvx tienvx force-pushed the remove-pact-broker branch 2 times, most recently from 4d2b1fc to 28128fc Compare May 8, 2023 16:27
@tienvx
Copy link
Contributor Author

tienvx commented May 11, 2023

it could become a separate PHP package

@YOU54F thanks for the idea. I will take it into account when this PR is being reviewed.

@YOU54F
Copy link
Member

YOU54F commented Jul 24, 2023

merged in #323

e401847

@tienvx
Copy link
Contributor Author

tienvx commented Jul 25, 2023

Close as there will be a PR from ffi to master

@tienvx tienvx closed this Jul 25, 2023
@tienvx tienvx deleted the remove-pact-broker branch July 31, 2023 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants